Use ASCII JSON responses for proxy compatibility#1645
Open
leovefy wants to merge 2 commits into
Open
Conversation
There was a problem hiding this comment.
Hey - 我发现了 1 个问题
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="module/server/app.py" line_range="24-33" />
<code_context>
from starlette.staticfiles import StaticFiles
+class ASCIIJSONResponse(JSONResponse):
+ """Serialize non-ASCII characters as ``\\uXXXX`` for proxy compatibility."""
+
+ def render(self, content) -> bytes:
+ return json.dumps(
+ content,
+ ensure_ascii=True,
+ allow_nan=False,
+ indent=None,
+ separators=(",", ":"),
+ ).encode("utf-8")
+
+
</code_context>
<issue_to_address>
**suggestion:** 建议复用 JSONResponse 的编码管线,而不是在这里直接硬编码调用 json.dumps。
使用直接调用 `json.dumps` 的方式重写 `render` 会绕过 JSONResponse 的编码管线(例如 `jsonable_encoder`、应用级自定义 `dumps`、未来切换到 `orjson` 等)。为了让这个响应与框架的其他部分保持一致,建议考虑委托给 `super().render`,只调整必要的参数(比如 `ensure_ascii`),或者重写 JSONResponse 在执行 dumps 时所使用的相关钩子,而不是重新实现完整的 render 逻辑。
建议实现:
```python
from pathlib import Path
import json
from contextlib import asynccontextmanager
from starlette.staticfiles import StaticFiles
from fastapi.responses import JSONResponse
from fastapi.encoders import jsonable_encoder
class ASCIIJSONResponse(JSONResponse):
"""Serialize non-ASCII characters as ``\\uXXXX`` for proxy compatibility."""
def render(self, content) -> bytes:
# Reuse FastAPI's JSON encoding pipeline (jsonable_encoder, etc.)
encoded_content = jsonable_encoder(content)
return json.dumps(
encoded_content,
ensure_ascii=True,
allow_nan=False,
indent=None,
separators=(",", ":"),
).encode("utf-8")
```
1. 如果你的项目已经在本文件其他位置导入了 `JSONResponse` 和/或 `jsonable_encoder`,请调整导入部分以避免重复导入,并保持一致的导入风格。
2. 如果你使用了自定义的 JSON dumps 函数(例如通过应用级配置或 `ORJSONResponse`),并且希望 `ASCIIJSONResponse` 也参与该配置,可以考虑:
- 在 `__init__` 中接受一个 `json_dumps` 可调用对象(默认使用应用配置的 dumps);
- 在 `render` 中使用该可调用对象替代标准的 `json.dumps`,并在可调用对象支持的前提下,通过关键字参数传入 `ensure_ascii=True`。
</issue_to_address>帮我变得更有用!请在每条评论上点 👍 或 👎,我会根据你的反馈改进后续评审。
Original comment in English
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="module/server/app.py" line_range="24-33" />
<code_context>
from starlette.staticfiles import StaticFiles
+class ASCIIJSONResponse(JSONResponse):
+ """Serialize non-ASCII characters as ``\\uXXXX`` for proxy compatibility."""
+
+ def render(self, content) -> bytes:
+ return json.dumps(
+ content,
+ ensure_ascii=True,
+ allow_nan=False,
+ indent=None,
+ separators=(",", ":"),
+ ).encode("utf-8")
+
+
</code_context>
<issue_to_address>
**suggestion:** Consider reusing JSONResponse’s encoding pipeline rather than hard-coding json.dumps here.
Overriding `render` with a direct `json.dumps` call bypasses JSONResponse’s encoding pipeline (e.g., `jsonable_encoder`, app-level custom `dumps`, future switches to `orjson`). To keep this response consistent with the rest of the framework, consider delegating to `super().render` and only adjusting the necessary parameters (such as `ensure_ascii`), or overriding the relevant hook used by JSONResponse for dumps instead of reimplementing the full render logic.
Suggested implementation:
```python
from pathlib import Path
import json
from contextlib import asynccontextmanager
from starlette.staticfiles import StaticFiles
from fastapi.responses import JSONResponse
from fastapi.encoders import jsonable_encoder
class ASCIIJSONResponse(JSONResponse):
"""Serialize non-ASCII characters as ``\\uXXXX`` for proxy compatibility."""
def render(self, content) -> bytes:
# Reuse FastAPI's JSON encoding pipeline (jsonable_encoder, etc.)
encoded_content = jsonable_encoder(content)
return json.dumps(
encoded_content,
ensure_ascii=True,
allow_nan=False,
indent=None,
separators=(",", ":"),
).encode("utf-8")
```
1. If your project already imports `JSONResponse` and/or `jsonable_encoder` elsewhere in this file, adjust the import section to avoid duplicates and keep a consistent import style.
2. If you use a custom JSON dumps function (e.g. via app-level configuration or `ORJSONResponse`), and you want `ASCIIJSONResponse` to participate in that configuration, consider:
- Accepting a `json_dumps` callable in `__init__` (defaulting to the app's configured dumps), and
- Using that callable instead of the standard `json.dumps` inside `render`, while still passing `ensure_ascii=True` via keyword arguments if the callable supports it.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Split from leovefy/cobra.
This PR contains a single commit: 28a71a9.
Base: runhey/dev.
Summary by Sourcery
将 FastAPI 应用配置为在所有响应和内部错误处理中使用仅包含 ASCII 字符的 JSON 响应类,以提升与代理的兼容性。
New Features:
ASCIIJSONResponse类,以仅使用 ASCII 字符序列化 JSON,从而兼容特定代理。Enhancements:
ASCIIJSONResponse,以确保输出的 JSON 一致。Original summary in English
Summary by Sourcery
Configure the FastAPI application to use an ASCII-only JSON response class for all responses and internal error handling to improve proxy compatibility.
New Features:
Enhancements: